[Trimming] Fix startup crash when TypeMapAssemblyTarget attribute survives but target assembly is fully trimmed - #130589
Conversation
…no surviving entries Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
….cs and assemblynative.cpp Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
|
@copilot No changes should be done to CoreCLR itself. This is entirely a trimming issue. When the trimmer decides to remove a type based on the typemap reference algorithm, it should also remove the typemap attributes that reference it. |
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Reverted all changes to
Reverted all changes to |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "0d63a4015f5a83231ef59e93277ff29fa2c82e1e",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "0e7b627c152a3c1885d1db498d81d01de55a3391",
"last_reviewed_commit": "0d63a4015f5a83231ef59e93277ff29fa2c82e1e",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "0e7b627c152a3c1885d1db498d81d01de55a3391",
"last_recorded_worker_run_id": "29682980805",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "0d63a4015f5a83231ef59e93277ff29fa2c82e1e",
"review_id": 4730589612
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: In trimmed CoreCLR apps a TypeMapAssemblyTarget attribute could survive trimming even when its named target assembly was fully trimmed away (all its TypeMap entries were conditional on trim targets that were never marked). At runtime TypeMapLazyDictionary unconditionally calls Assembly.Load for every surviving attribute, so the residual attribute caused a FileNotFoundException startup crash. This is a real, clearly motivated correctness fix.
Approach: TypeMapHandler no longer marks a TypeMapAssemblyTarget attribute unconditionally when its group is seen. Instead it resolves the target assembly during TypeMapResolver.Resolve, stores it alongside the pending entry, and defers the decision to MarkAssemblyTargetIfReady: the attribute is marked only if the target assembly is (or becomes) marked, dropped if the target assembly cannot be resolved, and deferred in _pendingAssemblyTargetsByAssembly otherwise. MarkStep.MarkAssembly flushes deferred entries via TriggerPendingAssemblyTargets when any assembly is first marked, which removes the visitation-order dependency the reviewer flagged. The over-approximation (keeping the attribute whenever the target assembly is marked for any reason, not strictly when it retains a surviving TypeMapAttribute) is a reasonable, well-commented tradeoff for a narrow case.
Summary: The change is correct, self-consistent, and low-risk. The new test TypeMapAssemblyTargetRemovedWhenAllEntriesTrimmed reproduces the scenario with a separate group-type assembly plus a conditional.dll referenced only by name string, and now asserts both [RemovedAssembly("conditional.dll")] and [RemovedAttributeInAssembly("test", typeof(TypeMapAssemblyTargetAttribute<AllConditionalGroupType>))], giving explicit verification of the intended behavior. Prior reviewer feedback from jtschuster (call TriggerPendingAssemblyTargets from MarkStep, remove the now-redundant belt-and-suspenders call, add the over-approximation comment) has all been incorporated. The pre-existing unvalidated ConstructorArguments[0] read in TypeMapResolver.Resolve was explicitly deferred by the maintainer to a separate change and is out of scope here. No actionable findings; the fix looks good.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 126.5 AIC · ⌖ 10.6 AIC · ⊞ 10K
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8d5de1a2-a6e8-46fb-84d2-6a971d76419c
|
@copilot please add Reflection.TypeMapAssemblyTargetRemovedWhenAllEntriesTrimmed to ILTrimExpectedFailures.txt. |
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Added
Added |
In trimmed CoreCLR apps, a
TypeMapAssemblyTargetattribute could survive trimming even when its named assembly was fully trimmed (all its TypeMap entries were conditional on trim targets that were never marked). At runtime,TypeMapLazyDictionaryunconditionally callsAssembly.Loadfor every surviving attribute, crashing withFileNotFoundException.Changes
Fix — ILLink
TypeMapHandler.csTypeMapAssemblyTargetattributes are now only marked when their target assembly is also marked (has surviving entries or is otherwise kept)._pendingAssemblyTargetsByAssembly: when a group is seen but the target assembly is not yet marked, the attribute is deferred here rather than immediately marked.TriggerPendingAssemblyTargets()isinternaland called only fromMarkStep.MarkAssemblywhenever an assembly is first marked — ensuring deferred attributes are flushed regardless of assembly visitation order.TypeMapAttribute, but the added complexity is not justified for a narrow case.Fix —
MarkStep.csMarkAssemblycalls_typeMapHandler.TriggerPendingAssemblyTargets(assembly)to flush any pendingTypeMapAssemblyTargetattributes for the assembly being marked, regardless of the order in which TypeMap entries are visited.Test
TypeMapAssemblyTargetRemovedWhenAllEntriesTrimmedwith two dependency assemblies: a shared group-type assembly, and aconditional.dllwith only conditional TypeMap entries (trim target never marked). Verifies via[RemovedAttributeInAssembly]and[RemovedAssembly]that theTypeMapAssemblyTargetattribute and the conditional assembly are both removed from the linked output.